Create, Delete, Enable, Disable, Enter, Cancel maintenance of Primary StoragePool with ONTAP storage#12563
Conversation
boring-cyborg
bot
added
component:build
component:primary-storage
component:storage
labels
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12563 +/- ##
============================================
- Coverage 18.01% 17.96% -0.06%
- Complexity 16459 16484 +25
============================================
Files 5968 6011 +43
Lines 537218 539998 +2780
Branches 65977 66182 +205
============================================
+ Hits 96801 96994 +193
- Misses 429498 432067 +2569
- Partials 10919 10937 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sandeeplocharla
force-pushed
the
netapp-ontap-primary-storage-pool
branch
2 times, most recently
from
c495c7c to
20d4e97
Compare
sandeeplocharla
force-pushed
the
netapp-ontap-primary-storage-pool
branch
from
20d4e97 to
c98cf8c
Compare
|
@blueorangutan package |
|
@kiranchavala a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 16683 |
|
@blueorangutan test |
|
@kiranchavala a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
Hey @sandeeplocharla et al, Great work, some issues came up in the github actions. Can you first look at the license check and the pre-commit actions? |
|
Hi @DaanHoogland |
Thank you, @DaanHoogland. We look forward to receiving your review comments and will address them as soon as possible. |
sandeeplocharla
force-pushed
the
netapp-ontap-primary-storage-pool
branch
2 times, most recently
from
08b53f0 to
b1792b4
Compare
|
Hi, the project level Code coverage seems to be failing, due to mutliple files that this code is indirectly touching, doesn't have coverage. What could be done in such a case? |
@sandeeplocharla , we have ambitions on coverage that we don’t meet historically. Please trty to meet them on your part of the code, more we cannot ask. As for the failing tests, these seem te be the same that are failing on main as well. If you can give us any hints on them that will be very much appreciated ;) |
There was a problem hiding this comment.
did you do license checks on the extra dependencies added in this file?
see https://apache.org/legal/resolved.html for explanation.
There was a problem hiding this comment.
Will check the licenses and get back to you.
There was a problem hiding this comment.
Hi @DaanHoogland , I've cross-checked the licenses of the dependencies added in our pom.xml. I had to remove one which wasn't conforming with ACF guidelines that you've shared.
Currently, all licenses in the ONTAP plugin are compliant with Apache CloudStack norms. All runtime dependencies use Apache License 2.0 (Category A). Test dependencies use MIT and EPL 2.0 licenses, which are also Category A compatible. No Category X licenses are present.
This PR does not have the benchmark coverage that we usually aim for within our team. But, we plan on having maximum code coverage possible with the upcoming PRs. As for this PR, there are around 5400 indirect files because of which the coverage dropped. I'm not sure, how we would able to handle them all. |
|
[SF] Trillian test result (tid-15359)
|
this is good enough until we encounter tangible issues. Please continue with the comment outstanding now. review effort will have to be made and I (or we) will hopefully get to this soon. Please be prepared for some rebasing and conflict resolving over the coming weeks 🥺 … unfortunately I cannot promise when further review will happen, but this PR is on my list. |
…ary StoragePool with ONTAP storage Co-authored-by: Rajiv Jain <Rajiv.Jain@netapp.com> Create & Delete, Enable & Disable, Enter & Cancel maintenance of Primary StoragePool with ONTAP storage Co-authored-by: Rajiv Jain<rajiv1@netapp.com> Edited readme file Fixed license check issue Removed dependency that's not conforming with ACF guidelines
sandeeplocharla
force-pushed
the
netapp-ontap-primary-storage-pool
branch
from
b1792b4 to
b8e5cd6
Compare
| throw new CloudRuntimeException("Provider name is null or empty, cannot create primary storage"); | ||
| } | ||
| s_logger.debug("ONTAP primary storage will be created as " + (managed ? "managed" : "unmanaged")); | ||
| if (!managed) { |
There was a problem hiding this comment.
this is not required, keep managed to true for ontap
| } | ||
|
|
||
| // Basic parameter validation | ||
| if (storagePoolName == null || storagePoolName.isEmpty()) { |
There was a problem hiding this comment.
| if (storagePoolName == null || storagePoolName.isEmpty()) { | |
| if (StringUtils.isBlank(storagePoolName)) { |
| if (storagePoolName == null || storagePoolName.isEmpty()) { | ||
| throw new CloudRuntimeException("Storage pool name is null or empty, cannot create primary storage"); | ||
| } | ||
| if (providerName == null || providerName.isEmpty()) { |
There was a problem hiding this comment.
| if (providerName == null || providerName.isEmpty()) { | |
| if (StringUtils.isBlank(providerName)) { |
| } else if (capacityBytes < ONTAP_MIN_VOLUME_SIZE) { | ||
| s_logger.warn("capacityBytes (" + capacityBytes + ") is below ONTAP minimum (" + ONTAP_MIN_VOLUME_SIZE + "), adjusting to minimum"); | ||
| capacityBytes = ONTAP_MIN_VOLUME_SIZE; | ||
| } |
There was a problem hiding this comment.
can do capacity checks in a separate method (maybe, getCapacityBytes), and return the capacityBytes? do it after parameter and scope validation.
@harikrishna-patnala Current PR has only changes scoped to as mentioned in scope of PR. Current ImplementationThe following operations are currently implemented:
We are ready with next set of changes which will cover CloudStack volume provisioning use cases for Netapp ONTAP. We can only raise our next PR once this PR gets merged. |
| Set<String> allowedKeys = new java.util.HashSet<>(requiredKeys); | ||
| allowedKeys.addAll(optionalKeys); | ||
|
|
||
| if (url != null && !url.isEmpty()) { |
There was a problem hiding this comment.
| if (url != null && !url.isEmpty()) { | |
| if (StringUtils.isNotBlank(url)) { |
| } | ||
| } | ||
|
|
||
| Set<String> providedKeys = new java.util.HashSet<>(details.keySet()); |
There was a problem hiding this comment.
import java.util.HashSet
Hi @harikrishna-patnala , except for addressing any mandatory comments, I'm not planning on adding any new features or code to this PR. |
| @Inject private StoragePoolAutomation _storagePoolAutomation; | ||
| @Inject private PrimaryDataStoreDao storagePoolDao; | ||
| @Inject private StoragePoolDetailsDao storagePoolDetailsDao; | ||
| private static final Logger s_logger = LogManager.getLogger(OntapPrimaryDatastoreLifecycle.class); |
There was a problem hiding this comment.
| private static final Logger s_logger = LogManager.getLogger(OntapPrimaryDatastoreLifecycle.class); | |
| private static final Logger logger = LogManager.getLogger(OntapPrimaryDatastoreLifecycle.class); |
| @Inject | ||
| private HostDao _hostDao; | ||
| @Inject private StoragePoolHostDao storagePoolHostDao; | ||
|
|
| private PrimaryDataStoreDao _storagePoolDao; | ||
| @Inject | ||
| private HostDao _hostDao; | ||
| @Inject private StoragePoolHostDao storagePoolHostDao; |
There was a problem hiding this comment.
| @Inject private StoragePoolHostDao storagePoolHostDao; | |
| @Inject | |
| private StoragePoolHostDao storagePoolHostDao; |
|
|
||
| @Override | ||
| public boolean hostDisconnected(long hostId, long poolId) { | ||
| return false; |
There was a problem hiding this comment.
@sandeeplocharla can move the above method implementation here, and above method can call this using the default implementation here:
| @Component | ||
| public class OntapPrimaryDatastoreProvider implements PrimaryDataStoreProvider { | ||
|
|
||
| private static final Logger s_logger = LogManager.getLogger(OntapPrimaryDatastoreProvider.class); |
There was a problem hiding this comment.
| private static final Logger s_logger = LogManager.getLogger(OntapPrimaryDatastoreProvider.class); | |
| private static final Logger logger = LogManager.getLogger(OntapPrimaryDatastoreProvider.class); |
|
|
||
|
|
||
| public class StorageProviderFactory { | ||
| private static final Logger s_logger = LogManager.getLogger(StorageProviderFactory.class); |
There was a problem hiding this comment.
| private static final Logger s_logger = LogManager.getLogger(StorageProviderFactory.class); | |
| private static final Logger logger = LogManager.getLogger(StorageProviderFactory.class); |
|
|
||
| private List<Aggregate> aggregates; | ||
|
|
||
| private static final Logger s_logger = LogManager.getLogger(StorageStrategy.class); |
There was a problem hiding this comment.
| private static final Logger s_logger = LogManager.getLogger(StorageStrategy.class); | |
| private static final Logger logger = LogManager.getLogger(StorageStrategy.class); |
| return false; | ||
| } | ||
| List<Aggregate> aggrs = svm.getAggregates(); | ||
| if (aggrs == null || aggrs.isEmpty()) { |
There was a problem hiding this comment.
| if (aggrs == null || aggrs.isEmpty()) { | |
| if (CollectionUtils.isEmpty(aggrs)) { |
| this.aggregates = List.of(aggr); | ||
| break; | ||
| } | ||
| if (this.aggregates == null || this.aggregates.isEmpty()) { |
There was a problem hiding this comment.
| if (this.aggregates == null || this.aggregates.isEmpty()) { | |
| if (CollectionUtils.isEmpty(this.aggregates)) { |
| s_logger.info("Creating volume: " + volumeName + " of size: " + size + " bytes"); | ||
|
|
||
| String svmName = storage.getSvmName(); | ||
| if (aggregates == null || aggregates.isEmpty()) { |
There was a problem hiding this comment.
| if (aggregates == null || aggregates.isEmpty()) { | |
| if (CollectionUtils.isEmpty(aggregates)) { |
| } | ||
| for (Aggregate aggr : aggrs) { | ||
| s_logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid()); | ||
| Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid()); |
| s_logger.warn("Aggregate " + aggr.getName() + " does not have sufficient available space. Required=" + | ||
| size + " bytes, available=" + availableBytes + " bytes. Skipping this aggregate."); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
can move aggrResp checks to a method, and use the same call in above method as well?
|
|
||
| public class UnifiedNASStrategy extends NASStrategy { | ||
|
|
||
| private static final Logger s_logger = LogManager.getLogger(UnifiedNASStrategy.class); |
There was a problem hiding this comment.
| private static final Logger s_logger = LogManager.getLogger(UnifiedNASStrategy.class); | |
| private static final Logger logger = LogManager.getLogger(UnifiedNASStrategy.class); |
| public Map<String, String> getLogicalAccess(Map<String, String> values) { | ||
| return null; | ||
| } | ||
|
|
|
|
||
| public class UnifiedSANStrategy extends SANStrategy { | ||
|
|
||
| private static final Logger s_logger = LogManager.getLogger(UnifiedSANStrategy.class); |
There was a problem hiding this comment.
| private static final Logger s_logger = LogManager.getLogger(UnifiedSANStrategy.class); | |
| private static final Logger logger = LogManager.getLogger(UnifiedSANStrategy.class); |
| } | ||
|
|
||
| public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details) { | ||
| if (details == null || details.isEmpty()) { |
There was a problem hiding this comment.
| if (details == null || details.isEmpty()) { | |
| if (MapUtils.isEmpty(details)) { |
|
|
||
| public class OntapStorageUtils { | ||
|
|
||
| private static final Logger s_logger = LogManager.getLogger(OntapStorageUtils.class); |
There was a problem hiding this comment.
| private static final Logger s_logger = LogManager.getLogger(OntapStorageUtils.class); | |
| private static final Logger logger = LogManager.getLogger(OntapStorageUtils.class); |
| public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details) { | ||
| if (details == null || details.isEmpty()) { | ||
| s_logger.error("getStrategyByStoragePoolDetails: Storage pool details are null or empty"); | ||
| throw new CloudRuntimeException("getStrategyByStoragePoolDetails: Storage pool details are null or empty"); |
There was a problem hiding this comment.
check/ensure that these method names in the errors are not passed to the user
| volume.setUuid("test-volume-uuid"); | ||
| volume.setName("testVolume"); | ||
| when(storageStrategy.createStorageVolume(any(), any())).thenReturn(volume); | ||
|
|
| | `password` | Yes | ONTAP cluster admin password | | ||
| | `svmName` | Yes | Storage Virtual Machine name | | ||
| | `protocol` | Yes | Storage protocol (`NFS3` or `ISCSI`) | | ||
| | `managementLIF` | Yes | ONTAP cluster management LIF IP address | |
There was a problem hiding this comment.
@sandeeplocharla does the UI accepts these parameters during create storage pool with ONTAP?
|
@sureshanaparti Thank you so much for taking time to review the PR. |
Create & Delete, Enable & Disable, Enter & Cancel maintenance of Primary StoragePool with ONTAP storage
Co-authored-by: Rajiv Jain rajiv1@netapp.com
Co-authored-by: Piyush Srivastava piyush5@netapp.com
Co-authored-by: Surya Gupta suryag@netapp.com
Description
NetApp ONTAP Storage Plugin
Introduction
This document describes in brief the design and implementation of the NetApp ONTAP storage plugin for Apache CloudStack. The plugin enables CloudStack to use NetApp ONTAP as a primary storage provider.
ONTAP Terminology
Scope
Current Implementation
The following operations are currently implemented:
Supported Configurations
Storage Pool Lifecycle
Create Storage Pool
Attach to Cluster/Zone
Delete Storage Pool
Configuration Parameters
Parameters are passed via URL in semicolon-separated key=value format:
usernamepasswordmanagementLIFsvmNameprotocolNFS3orISCSIThere are few files under feign folder which aren't currently being consumed by the workflows being proposed for review. But, they are definitely required for upcoming workflows.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
How Has This Been Tested?
Management Server Operations:
Primary Storage Pool Operations:
Testing Environment:
NFS3 Primary Storage Pool - CreateNFS3 type ONTAP Volume createdExport Policy created on ONTAPDisabled Primary Storage Pool - NFS3Re-Enabled Primary Storage Pool - NFS3Enter maintenance mode - NFS3Cancel maintenance mode - NFS3Delete Primary StoragePool - NFS3ONTAP Volume deletedONTAP ExportPolicy deletediSCSI Primary Storage Pool - DeleteiSCSI type ONTAP Volume creatediGroup createdDisabled Primary Storage Pool - iSCSIRe-Enabled Primary Storage Pool - iSCSIEnter maintenance mode - iSCSICancel maintenance mode - iSCSIDelete Primary StoragePool - iSCSIONTAP iGroup deletedONTAP Volume deleted